Desktop: Fixes #9985: Filter Sync Target Info Logs #10014
Conversation
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
|
The test fails due to a issue with cspell in the unit test, should I just disable it for this test? |
|
@PackElend label me please Introduction URL: https://discourse.joplinapp.org/t/introducing-criticic/36240 |
| if (mk.content) delete mk.content; | ||
| if (mk.checksum) delete mk.checksum; |
There was a problem hiding this comment.
I remember some tests failing due to this, though they run fine now without it also. Not sure what caused the issue then.
There was a problem hiding this comment.
Ok because I think with real data mk.checksum is an empty string so you wouldn't delete it in this case. And this check is unnecessary anyway
| ppk_: { | ||
| value: { | ||
| // Public Key is truncated to 40 characters | ||
| publicKey: 'longstringverylongstringlongstringverylo', |
There was a problem hiding this comment.
Would you mind adding "..." to this string too to show it's truncated?
There was a problem hiding this comment.
And please remove all the comments. They are not necessary and are likely to become outdated.
| activeMasterKeyId_: originalSyncInfo.activeMasterKeyId, | ||
| appMinVersion_: originalSyncInfo.appMinVersion, | ||
| e2ee_: originalSyncInfo.e2ee, | ||
| version_: originalSyncInfo.version, |
There was a problem hiding this comment.
When you create a test you should not use dynamic data, especially not data that comes from the data you're supposed to check. So all these should be static strings/numbers.
| createdTime: 0, | ||
| keySize: 0, | ||
| }, | ||
| updatedTime: originalSyncInfo.ppk.updatedTime, |
| createdTime: 0, | ||
| keySize: 0, |
There was a problem hiding this comment.
Could you try the app and use more realistic data? Checking that the time or size is 0 is not a good test - it might be 0 because of a bug.
|
Please use this as test data: But really you should have looked for this data yourself or asked around. Testing with fake data with 0 values and empty strings is not something we can rely on. |
|
In this case I had first made it all static data (from the running Joplin App), but then decided to write it this way. (seemed cleaner to my brain) but yeah I agree it might not be suited for a test |
|
I think now this is in line with the feedback you provided |
| // Truncate the private key and public key | ||
| if (filtered.ppk_.value) { | ||
| filtered.ppk_.value.privateKey.ciphertext = `${filtered.ppk_.value.privateKey.ciphertext.substr(0, 20)}...${filtered.ppk_.value.privateKey.ciphertext.substr(-20)}`; | ||
| filtered.ppk_.value.publicKey = `${filtered.ppk_.value.publicKey.substr(0, 20)}...${filtered.ppk_.value.publicKey.substr(-20)}`; |
There was a problem hiding this comment.
Not what was asked. The point of including the first 40 characters is to show the start of the public key data. With your change all we get is "-----BEGIN RSA PUBLI... RSA PUBLIC KEY-----" which is useless.
There was a problem hiding this comment.
sorry for the misunderstanding, have corrected it
Fixes #9985 .